-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix #12530: Wildcard of value types don't cover null #12533
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
||
case Ident(nme.WILDCARD) => | ||
Or(Typ(erase(pat.tpe.stripAnnots), false) :: constantNullSpace :: Nil) | ||
if pat.tpe <:< defn.AnyValType then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we use isNullableClass instead? That way null won't pollute our patterns when -Yexplicit-nulls is enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, for exhaustiveness we don't report missing nulls, so maybe it's fine to do the same for unreachability?
scala> final case class Foo(x: Int)
// defined case class Foo
scala> def foo(x: Foo) = x match { case Foo(_) => }
def foo(x: Foo): Unit
scala> foo(null)
scala.MatchError: null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually I guess we should emit a warning _ only matches null
like we already do for case _ =>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a feature that reports a warning if the last wildcard only matches null
(#4253 (comment)). I think removing the null
related stuff will make the code simpler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually I guess we should emit a warning
_ only matches null
like we already do forcase _ =>
That would make the check more complex and expensive, while not very useful to users. I think removing null
related logic will produce expected and helpful warnings to users while keeping the logic simple and fast.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, we can avoid computing precisely whether a pattern covers exactly null
. Instead, we can blindly provide a hint for null
if the last case is _
and is unreachable. This allows us to use Typ(tp)
instead of Or(Type(tp), nullSpace)
for a wildcard pattern.
Doing that simplifies the code, and uncovers a bug that I'm now investigating:
object Test {
sealed trait Cause[+E]
object Cause {
final case class Fail[E](value: E) extends Cause[E]
}
def fn(cause: Cause[Any]): String =
cause match {
case Cause.Fail(t: Throwable) => t.toString
case Cause.Fail(any) => any.toString
}
}
Previously no warning is produced because the checker thought that 2nd case matches Fail(null)
, which is incorrect.
case (Some(false), _) => | ||
case (Some(true), _) => | ||
case (None, _) => | ||
case (_, false) => // reachable: null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We now issue a warning here (see the check file).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise LGTM
Co-authored-by: Guillaume Martres <[email protected]>
Fix #12530: Wildcard of value types don't cover null